-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix EncryptedFile lacking mimetype #2581
Fix EncryptedFile lacking mimetype #2581
Conversation
From the room: it's somewhat unknown if this is an implementation bug bleeding into the spec ( https://github.com/matrix-org/matrix-doc/issues/2374 ) or an omission. |
MSC to remove it: #2582 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also a changelog please :)
@@ -299,6 +299,7 @@ hashes {string: string} **Required.** A map from an algorithm name to a hash | |||
``sha256``. | |||
v string **Required.** Version of the encrypted attachments | |||
protocol. Must be ``v2``. | |||
mimetype string The mimetype of the file, e.g. ``image/jpeg`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like adding this as a required field, when the intention is to remove it again later. That will just break interaction between clients, that implemented this according to the current spec, which is missing the mimetype. If this should be added as a required field, this should go through a MSC imo, since it will break client interactions. I don't see, why the original intention being different, which isn't really documented anywhere publicly from what I can tell, should remove the need for an MSC. Last time that was done (#1939), it caused multiple bug reports and complaints against my client, because my client followed the spec, but the spec was changed, when I wasn't looking (and the example wasn't updated). If this should be added to the spec, this would also need an explanation regarding what should be sent as the mimetype. Should it be the mimetype of the file after decryption or should it be "application/octet-stream", which matches the encrypted blob? It would also need to be documented, how conflicts with the info objects should be handled. If my client sends 2 different mimetypes in info and EncryptedFile, is the event considered invalid, the one from info taken or from EncryptedFile? Also how should events be handled, that don't have this required field? Should clients just not decrypt such media anymore just because the spec was changed? Since I got burned by such changed a few times already, I would prefer, if this doesn't get merged without an MSC, even though it would fix the spec issue I actually raised initially (#2374). After a bit of discussion in #matrix-spec I chose to not send mimetype and and opened that issue, but I don't remember, if I was encouraged to do that or I misunderstood/decided that myself. |
Let's just leave this as an omission then until the MSC gets resolved in some way. |
The example already had the mimetype parameter, but the definition lacked it.
Signed-off-by: Sorunome mail@sorunome.de